Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(ReferenceApiController): Bump rate limit for public resolve endpoint #49801

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mejo-
Copy link
Member

@mejo- mejo- commented Dec 11, 2024

E.g. text documents might contain hundreds of links whose previews need to get loaded.

Fixes: nextcloud/collectives#1607

Checklist

E.g. text documents might contain hundreds of links whose previews need
to get loaded.

Fixes: nextcloud/collectives#1607

Signed-off-by: Jonas <[email protected]>
@mejo- mejo- added bug 3. to review Waiting for reviews labels Dec 11, 2024
@mejo- mejo- self-assigned this Dec 11, 2024
@mejo-
Copy link
Member Author

mejo- commented Dec 11, 2024

/backport to stable30

@blizzz blizzz requested a review from nickvergessen December 11, 2024 15:02
@blizzz blizzz added this to the Nextcloud 31 milestone Dec 11, 2024
@@ -128,7 +128,7 @@ public function resolveOne(string $reference): DataResponse {
*/
#[ApiRoute(verb: 'GET', url: '/resolvePublic', root: '/references')]
#[PublicPage]
#[AnonRateLimit(limit: 10, period: 120)]
#[AnonRateLimit(limit: 200, period: 120)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#[AnonRateLimit(limit: 200, period: 120)]
#[AnonRateLimit(limit: 200, period: 3600)]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would mean that only 200 requests are allowed in 3600 seconds, which means 60 minutes, no? So reloading a document with 150 previews once would already hit the rate limit. Given that we cache the references internally I would prefer a shorter period.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the tricky part is that we do not cache invalid responses, so you can still issue a lot more requests, though I find it hard to come up with a reasonable amount there.

We could also think about doing lazy loading on the frontend side there as an additional step to only fetch the ones that are visible in the browsers viewport, but even with that 10 in 120 seconds might be to low for examples like the one shared in the collectives ticket.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

429: Too Many Requests
4 participants